Fix possible crash in validator when 'client' key is mentioned in request body #1252
Conversation
realsuayip
changed the title
There was a problem hiding this comment.
@realsuayip Thanks for the PR. I have two asks.
- Can you dig into why
request.clientwas set to something else? This could be an interactive custom code from your project, a conflict with package, or a symptom of a bug else where in oauth_toolkit or oauth_lib. I'd like us to be sure of what we are fixing. - We need to include a test to help avoid regressions.
n2ygk
force-pushed
the
fix-crash-token-obtain
branch
from
eda18ca to
1c74222
Compare
Codecov Report
@@ Coverage Diff @@
## master #1252 +/- ##
=======================================
Coverage 97.29% 97.29%
=======================================
Files 31 31
Lines 1996 1997 +1
=======================================
+ Hits 1942 1943 +1
Misses 54 54
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
In In order to create this instance there are multiple parameters required, one of them being Essentially, The problem is, Solutions that come to my mind: 1 - The solution in current PR, which checks for There might other internal attributes which might be affected from this (in which case solution 2 would be better), I noticed So, what do you think we should do? |
|
Well it seems like the simple solution is to correct your typo.... However, you are right DOT could provide a better Developer Experience here. Given your case, you intended to pass a client_id, but instead passed client. It could very well be that you didn't want the user to be logged into whatever the fallback client_id. That would be an erroneous situation that if we let it happen might created more subtle and harder to diagnose problems for you or your front end teams depending on your IDp. I don't think falling back to a default as I believe this PR is proposing would be a good solution because of that. I could be misinterpreting the flow here though. In theory the inputs to these endpoints should be restricted to those in the specification. Along that line of thinking any extraneous input could be considered an error, especially so if it conflicts with an internally reserved property on the request like in this case. I think it would be better to validate the input and raise an error if we find a param that conflicts with a reserved property like client. Alternatively we could silently ignore unknown properties as you proposed, but that could also lead to unexpected results for our users... although another lay of validation may handle that.... @realsuayip do validation errors seems like a reasonable course of action to you, or you do you have other thoughts based on my feedback? |
Yes, I think it's totally acceptable.
Does that mean we would only raise validation errors in case there is a conflict? If so, thats a bit odd. Maybe we should consider raising errors if given key is not valid.
Yes, besides, my main concern is exposing an endpoint where you can consistently crash the server. Some guy might just spam this endpoint with invalid requests just to deplete Sentry quotas 😁 |
I feel there are valid reasons to pass other arguments like ?utm_* tags for marketing and activity contribution, or a GA session id from a cookie to use GA measurement protocol server side to track a token issue event and associate it with a users session. Or maybe transactions ids to associate the span on the token issuance with a user session for debugging. For that reason I would lean toward of conservative of approach of only dis-allowing things we know will cause errors and conflict with our code.
That sounds like a reason to fix your typo. ;) |
To be clear, any Django application using |
That was meant as a joke. The rest of the feedback stands. I'm not going to approve a solution that overwrites the client property in this case. We should validate the input property that is conflicting and only that property and we should allow other query parameters for use cases such as analytics attribution and span tracking for telemetry. |
|
@dopry @realsuayip Where do we stand with this? Should I close it or @realsuayip will you be making the changes suggested by @dopry? |
|
@n2ygk I think we should leave it open or convert it an issue if @realsuayip doesn't have the bandwidth. It is after all a bug we need to address. |
dopry
force-pushed
the
fix-crash-token-obtain
branch
from
1c74222 to
472656c
Compare
|
@n2ygk I added the tests for this and updated the _load_application implementation. I'd appreciate a quick second review if you can find a moment. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
dopry
force-pushed
the
fix-crash-token-obtain
branch
from
a88b2f8 to
ca69d09
Compare
dopry
force-pushed
the
fix-crash-token-obtain
branch
from
ca69d09 to
6e9912a
Compare
dopry
force-pushed
the
fix-crash-token-obtain
branch
from
6e9912a to
704885b
Compare
instead of the client_id
instead of the client_id, cleaner implementation
dopry
force-pushed
the
fix-crash-token-obtain
branch
from
704885b to
611cd52
Compare
|
@realsuayip if you have time to test my changes to the PR I'd appreciate it. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the _load_application method in the OAuth2 validator to implement caching logic and improve error handling. The changes replace an assertion-based approach with a more robust client validation mechanism that checks cached clients before performing database lookups.
Key Changes
- Replaced assertion check with conditional logic to handle cached
request.clientinstances - Added comprehensive validation for cached clients (type checking, client_id matching, and usability checks)
- Improved logging messages throughout the method for better debugging
- Added extensive test coverage for the refactored
_load_applicationmethod
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| oauth2_provider/oauth2_validators.py | Refactored _load_application to implement client caching logic with validation and improved logging |
| tests/test_oauth2_validators.py | Added 7 new test cases covering various scenarios of the refactored _load_application method including cache hits, mismatches, and usability checks |
| tests/test_authorization_code.py | Added test case to verify error handling when using incorrect parameter name (client instead of client_id) in token request |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert hasattr(request, "client"), '"request" instance has no "client" attribute' | ||
|
|
||
| if request.client: | ||
| """ check for cached client, to save the db hit if this has alredy been loaded """ |
There was a problem hiding this comment.
Corrected spelling of 'alredy' to 'already'.
| """ check for cached client, to save the db hit if this has alredy been loaded """ | |
| """ check for cached client, to save the db hit if this has already been loaded """ |
| if request.client: | ||
| """ check for cached client, to save the db hit if this has alredy been loaded """ | ||
| if not isinstance(request.client, Application): | ||
| log.debug("request.client is not an Application, something else set request.client erroroneously, resetting request.client.") |
There was a problem hiding this comment.
Corrected spelling of 'erroroneously' to 'erroneously'.
| log.debug("request.client is not an Application, something else set request.client erroroneously, resetting request.client.") | |
| log.debug("request.client is not an Application, something else set request.client erroneously, resetting request.client.") |
| assert hasattr(request, "client"), '"request" instance has no "client" attribute' | ||
|
|
||
| if request.client: | ||
| """ check for cached client, to save the db hit if this has alredy been loaded """ |
There was a problem hiding this comment.
Use double quotes for docstrings instead of triple-quoted strings for inline comments. Change to a regular comment using # or convert to a proper multi-line docstring.
| # Check that the application can be used (defaults to always True) | ||
| if not request.client.is_usable(request): | ||
| log.debug("Failed body authentication: Application %r is disabled" % (client_id)) | ||
| """ cache wasn't hit, load from db """ |
There was a problem hiding this comment.
Use double quotes for docstrings instead of triple-quoted strings for inline comments. Change to a regular comment using # or convert to a proper multi-line docstring.
| """ cache wasn't hit, load from db """ | |
| # cache wasn't hit, load from db |
3 participants
Description of the Change
When sending a request to
oauth2_provider.views.base.TokenView, ifclientis sent via request body (typo ofclient_id), the server crashes with:'str' object has no attribute 'is_usable'(str object being
clientin request body)I'm not sure why
request.clientis set to this attribute; I'm guessing request body is added to oauth request as attrs.I changed the check so that application would be fetched if the type does not match.
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS